Skip to content

Riverlea rollout - set as the default theme from 6.0 (but freeze on Greenwich for existing installs using Greenwich) - #31979

Closed
ufundo wants to merge 5 commits into
civicrm:6.0from
ufundo:riverlea-rollout
Closed

Riverlea rollout - set as the default theme from 6.0 (but freeze on Greenwich for existing installs using Greenwich)#31979
ufundo wants to merge 5 commits into
civicrm:6.0from
ufundo:riverlea-rollout

Conversation

@ufundo

@ufundo ufundo commented Feb 6, 2025

Copy link
Copy Markdown
Contributor

Overview

This effects the switch from Greenwich to Riverlea as the default theme from 6.0.

Before

  • Greenwich is always installed, hidden, and the default theme
  • Riverlea is installed by default on Standalone, but not on other CMSes
  • Riverlea extension may or may not be installed on sites upgrading

After

  • Riverlea extension is required (installed on upgrade or new install). This does not mean it's always selected as the active theme, but its always available as theme option.
  • pre-existing sites with "default"/"automatic" selected for their theme at the point of upgrade will be tweaked so they have Greenwich explicitly selected (ie if you were getting Greenwich, you'll still have Greenwich)
  • new sites, and any users switching to "default" selection will get Riverlea Minetta (the Greenwich lookalike)
  • Greenwich is unhidden, so you can disable it in the UI (and you will then fallback to Riverlea if you were on Greenwich before)

@civibot

civibot Bot commented Feb 6, 2025

Copy link
Copy Markdown

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot Bot added the master label Feb 6, 2025
@ufundo
ufundo marked this pull request as ready for review February 6, 2025 16:59
@totten

totten commented Feb 7, 2025

Copy link
Copy Markdown
Member

+1 for auto-enabling on new installations

+1 for locking-in the setting on upgraded sites for continuity.

I suspect the test failures are more around swapping the interpretation of default and/or re-arranging other metadata -- which sounds nice, but isn't as urgent. (That kind of thing feels like it would be better at the start of a cycle rather than the end.)

The patch sort of swaps the hard-dependency in Civi\Core\Themes from greenwich to riverlea. I would tempted to relax that dependency, eg master...totten:civicrm-core:master-theme-default

@totten

totten commented Feb 7, 2025

Copy link
Copy Markdown
Member

Possible alternative: #31985

@ufundo

ufundo commented Feb 7, 2025

Copy link
Copy Markdown
Contributor Author

I had a higher level thought...

In the hope of explaining, lets distinguish between:

  • the Civi::settings "default" for 'theme_frontend' and 'theme_backend'
  • the stuff in Civi::themes called DEFAULT_THEME which corresponds setting the value of those settings to default. lets call that 'automatic' instead (thats the label in display settings)

Now, automatic purports to be "get the latest and greatest theme at the discretion of civicrm.org". But the first time in 5(?) years that latest amd greatest is changing, we arent actually exercising that discretion.

So... maybe we just get rid of Automatic? The other PR would mean new installs have an explicit theme choice, we want to flick people who were on Automatic to explicit Greenwich anyway for safety. I dont think an explicit theme choice is likely to pick Automatic (except in the spirit of "I wonder what this does")

So rather than adding more code to automatic I would suggest removing entirely. Migrate people currently on Automatic to Greenwich. Switch the setting level default to minetta. One less option in the UI, one less layer of default (over the settings default), one less line in the settings meta that says 'default' => 'default'

There's already the fallback handling which seems to work ok if your theme setting doesnt correspond to an theme that exists. (Your proposed change looks to me like it belongs more in fallback than default)

@ufundo
ufundo changed the base branch from master to 6.0 February 7, 2025 10:36
@civibot civibot Bot added 6.0 and removed master labels Feb 7, 2025
@ufundo ufundo closed this Feb 7, 2025
@ufundo

ufundo commented Feb 7, 2025

Copy link
Copy Markdown
Contributor Author

The patch sort of swaps the hard-dependency in Civi\Core\Themes from greenwich to riverlea. I would tempted to relax that dependency,

100% that makes more sense. I like the idea of basically removing it because then we don't actually need any required theme extensions. If people want to remove Greenwich and Riverlea, that's kinda reasonable from a system perspective.

@ufundo

ufundo commented Feb 7, 2025

Copy link
Copy Markdown
Contributor Author

Closing in favour of #31986

@ufundo

ufundo commented Feb 7, 2025

Copy link
Copy Markdown
Contributor Author

(Just keeping discussion on this PR so its mainly in one place)

There's already the fallback handling which seems to work ok if your theme setting doesnt correspond to an theme that exists. (Your proposed change looks to me like it belongs more in fallback than default)

I'd misunderstood how this was working. But I looked at a bit more and I think ufundo#9 clears it up and might be the simplest neatest way to handle "you've picked a theme which doesn't exist".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants